Skip to content

C++: Prevent an Expr from stepping to itself in IR dataflow#11640

Merged
MathiasVP merged 2 commits intogithub:mathiasvp/replace-ast-with-ir-use-usedataflowfrom
MathiasVP:local-expr-step-should-step
Dec 9, 2022
Merged

C++: Prevent an Expr from stepping to itself in IR dataflow#11640
MathiasVP merged 2 commits intogithub:mathiasvp/replace-ast-with-ir-use-usedataflowfrom
MathiasVP:local-expr-step-should-step

Conversation

@MathiasVP
Copy link
Copy Markdown
Contributor

@MathiasVP MathiasVP commented Dec 9, 2022

While investigating some query changes, I noticed that it can be the case that a single local step can step from a node n1 representing the an expression, to a node n2 that represents the expression's first conversion. Because n.asExpr then resolves to the unconverted expression, the result will be an expression e such that localExprFlowStep(e, e) holds 😱.

The fix is to add another level of recursion that ensures that we always step to a different expression. This is done in a328565. 52bf39b fixes a performance shown by the join-order detection in DCA. It turns out the * in the localExprFlow relation wasn't turned into a fastTC by the compiler, and it resulted in quite a few iterations that looked like:

Evaluated recursive predicate #DataFlowUtil#47741e1f::ExprFlowCached::localExprFlowStep#2Plus#bf@aafb49jn in 916ms on iteration 2 (delta size: 844629).
Evaluated relational algebra for predicate #DataFlowUtil#47741e1f::ExprFlowCached::localExprFlowStep#2Plus#bf@aafb49jn on iteration 2 running pipeline standard with tuple counts:
    923848     ~0%    {2} r1 = SCAN #DataFlowUtil#47741e1f::ExprFlowCached::localExprFlowStep#2Plus#bf#prev_delta OUTPUT In.1, In.0
  23112681  ~1725%    {2} r2 = JOIN r1 WITH DataFlowUtil#47741e1f::ExprFlowCached::localExprFlowStep#2#ff ON FIRST 1 OUTPUT Lhs.1, Rhs.1
    928088    ~14%    {2} r3 = r2 AND NOT #DataFlowUtil#47741e1f::ExprFlowCached::localExprFlowStep#2Plus#bf#prev(Lhs.0, Lhs.1)
                      return r3

So the last commit transforms that * into a fastTC manually.

@MathiasVP MathiasVP requested a review from a team as a code owner December 9, 2022 14:28
@github-actions github-actions Bot added the C++ label Dec 9, 2022
@MathiasVP MathiasVP added the no-change-note-required This PR does not need a change note label Dec 9, 2022
jketema
jketema approved these changes Dec 9, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@jketema jketema left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM assuming DCA is happy.

@MathiasVP
Copy link
Copy Markdown
Contributor Author

MathiasVP commented Dec 9, 2022

DCA looks fine 🎉

@MathiasVP MathiasVP merged commit 2ad61df into github:mathiasvp/replace-ast-with-ir-use-usedataflow Dec 9, 2022
@jketema
Copy link
Copy Markdown
Contributor

jketema commented Dec 9, 2022

More than fine 😄

@MathiasVP MathiasVP mentioned this pull request Dec 16, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jketema jketema jketema approved these changes

Assignees

No one assigned

Labels

C++ no-change-note-required This PR does not need a change note

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@MathiasVP @jketema